Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New command: app:new:from:drupal7 #1543

Merged
merged 34 commits into from
Aug 10, 2023
Merged

New command: app:new:from:drupal7 #1543

merged 34 commits into from
Aug 10, 2023

Conversation

wimleers
Copy link
Member

Motivation
ACLI already has a app:new:local command that spins up a brand new Drupal project.

Wouldn't it be great if it also helped you spin up a new Drupal 9+ project that helps migrating from Drupal 7?

Proposed changes
Introduce acli app:new:from:drupal7.

All code has been battle-hardened over years of use in production on Acquia Cloud. Please check the commits in sequence, because I designed them to tell a gradual story:

  1. code to inspect a Drupal 7 site's installed modules
  2. introduce the new ACLI command
  3. add unit tests
  4. add integration tests

Until the full set of recommendations is released, this will use only a minimal set of recommendations, primarily consisting of some Drupal 7 → 9 migration recommendations for Acquia modules.

This is a necessary step in the way of open sourcing https://www.acquia.com/products/drupal-cloud/acquia-migrate-accelerate.

Alternatives considered
N/A

Testing steps
Point it at a Drupal 7 site and observe that it generates a valid composer.json that results in an installable Drupal 9+ site. See the integration tests for various sample Drupal 7 sites at tests/phpunit/src/Commands/App/NewFromDrupal7CommandTest.php, with fixtures at tests/fixtures/drupal7.

Gabe Sullice and others added 19 commits June 15, 2023 17:01
…s to actually test the full range of possibilities.
…o bootstrap Drupal 7. That cannot happen int ests, so adjust the logic in `NewFromDrupal7Command` slightly.
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Patch coverage: 91.61% and project coverage change: -0.02% ⚠️

Comparison is base (fe86843) 91.77% compared to head (c214e45) 91.76%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1543      +/-   ##
============================================
- Coverage     91.77%   91.76%   -0.02%     
- Complexity     1663     1809     +146     
============================================
  Files           110      124      +14     
  Lines          5957     6470     +513     
============================================
+ Hits           5467     5937     +470     
- Misses          490      533      +43     
Files Changed Coverage Δ
...pp/From/Recommendation/UniversalRecommendation.php 75.00% <75.00%> (ø)
src/Command/App/NewFromDrupal7Command.php 77.24% <77.24%> (ø)
...mmand/App/From/Safety/StructuredArrayValidator.php 84.37% <84.37%> (ø)
...c/Command/App/From/Safety/ArrayValidationTrait.php 90.90% <90.90%> (ø)
.../From/Recommendation/AbandonmentRecommendation.php 96.42% <96.42%> (ø)
src/Command/App/From/Composer/ProjectBuilder.php 100.00% <100.00%> (ø)
src/Command/App/From/Configuration.php 100.00% <100.00%> (ø)
src/Command/App/From/JsonResourceParserTrait.php 100.00% <100.00%> (ø)
.../App/From/Recommendation/DefinedRecommendation.php 100.00% <100.00%> (ø)
...ommand/App/From/Recommendation/Recommendations.php 100.00% <100.00%> (ø)
... and 4 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anavarre
Copy link
Contributor

Hey Wim! Thanks for your contribution. @danepowell is out for ~2 weeks so expect radio silence for a little while. Making visible to @mikemadison13 for AMA efforts.

@wimleers
Copy link
Member Author

Hey Wim! Thanks for your contribution. @danepowell is out for ~2 weeks so expect radio silence for a little while. Making visible to @mikemadison13 for AMA efforts.

Yep, I'm aware 😊

@wimleers
Copy link
Member Author

The job was canceled because "ubuntu-22_04_8_1_pcov" failed.

🤷 Looks like GitHub Actions is broken today…

@wimleers
Copy link
Member Author

At least in 987ce00, there were some genuine failures:

1)                                                                     
         Acquia\Cli\Tests\Commands\App\From\AbandonmentRecommendationTest::testVersionCon
         straint                                                                
         "@covers getVersionConstraint" is invalid      

Fixed in 9a0814b.

Rooting for full green again after the weird auto-canceling CI jobs! 🤞

…pal7SiteInspector` to increase the reported code coverage; these only operate on live Drupal 7 sites so are by definition untestable.
@wimleers
Copy link
Member Author

Green again!

Just "failing" on reduced code coverage: from 91.56% for the entire project to 90.93%.

From 81.31% in 5ec7893, to 82.55% in 9a0814b, to 84.17% test coverage in e22a60b for new code. I have one more idea to get it up significantly, just pushed 7d88c51 for that 🤞

@wimleers
Copy link
Member Author

🥳 91.58% of target 91.56% 😄

@wimleers
Copy link
Member Author

Aha, looks like there was a failure on Windows too. getLocation() only works on UNIX-style paths today:

Script @unit-parallel was called via unit
1) Acquia\Cli\Tests\Commands\App\NewFromDrupal7CommandTest::testNewFromDrupal7Command with data set "training.acquia.com" ('D:\a\cli\cli/tests/fixtures/d...s.json', 'D:\a\cli\cli/config/from_d7_r...s.json', 'D:\a\cli\cli/tests/fixtures/d...d.json')
Script @unit was called via test
TypeError: Acquia\Cli\Command\App\NewFromDrupal7Command::getLocation(): Return value must be of type string, bool returned

Attempted a fix.

…paths on Windows: it fails on `D:/a/cli/cli/config/…`.
@wimleers
Copy link
Member Author

  • before a694e0e: D:\a\cli\cli/tests/fixtures/
  • after a694e0e: D:/a/cli/cli/tests/fixtures/

👆 Now paths were formatted in a way accepted by Windows. But our logic to detect absolute paths vs not was wrong. That's what da056f6 is aiming to fix.

@wimleers
Copy link
Member Author

🥳 Green!

For fun, I also checked the mutation testing output, which I've never before seen. It's pretty cool, and it shows that the tests catch most things:

279 mutations were generated:
     216 mutants were killed
       0 mutants were configured to be ignored
       0 mutants were not covered by tests
      62 covered mutants were not detected
       0 errors were encountered
       0 syntax errors were encountered
       1 time outs were encountered
       0 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 77%
         Mutation Code Coverage: 100%
         Covered Code MSI: 77%

Most of these are for the untestable "inspect a live Drupal 7 site" code path. But not all. One of them is pretty silly and is only due to a limitation in PHP <8:

Warning: Escaped Mutant for Mutator "DecrementInteger":

--- Original
+++ New
@@ @@
     {
         assert(is_resource($resource));
         $json = stream_get_contents($resource);
-        return json_decode($json, TRUE, 512, JSON_OBJECT_AS_ARRAY | JSON_THROW_ON_ERROR);
+        return json_decode($json, TRUE, 511, JSON_OBJECT_AS_ARRAY | JSON_THROW_ON_ERROR);
     }
 }


Warning: Escaped Mutant for Mutator "IncrementInteger":

--- Original
+++ New
@@ @@
     {
         assert(is_resource($resource));
         $json = stream_get_contents($resource);
-        return json_decode($json, TRUE, 512, JSON_OBJECT_AS_ARRAY | JSON_THROW_ON_ERROR);
+        return json_decode($json, TRUE, 513, JSON_OBJECT_AS_ARRAY | JSON_THROW_ON_ERROR);
     }
 }

Just switching to PHP >=8's named arguments fixes this!

@wimleers
Copy link
Member Author

wimleers commented Jun 19, 2023

Entire codebase with this PR:

3646 mutations were generated:
    1364 mutants were killed
       0 mutants were configured to be ignored
       0 mutants were not covered by tests
     772 covered mutants were not detected
      69 errors were encountered
       0 syntax errors were encountered
      20 time outs were encountered
    1421 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 65%
         Mutation Code Coverage: 100%
         Covered Code MSI: 65%

☝️ this PR means a net improvement to the mutation score per my previous comment: ~77% vs ~65%. 😊 (Would be cool to have a target to not regress against this too?)

@wimleers
Copy link
Member Author

a9c042e fixes a bug that I found while updating an integration test of a private repo to rely on the new ACLI command being introduced here.

That integration test does point to a real, live Drupal 7 site. It crashed like so:

php ./bin/acli app:new:from:drupal7 --drupal7-directory $D7_DIR --recommendations "$RECOMMENDATIONS_DIR/recommendations.json" --directory /tmp/build
We strive to give you the best tools for development.
You can really help us improve by sharing anonymous performance and usage data.
 Would you like to share anonymous performance usage and data? (yes/no) [yes]:
 > Awesome! Thank you for helping!

 [ERROR] The given --drupal7-uri value does not correspond to an installed sites
         directory and a sites.php file could not be located.        

@danepowell
Copy link
Contributor

@wimleers this looks good to me, but it's still in draft state. Is this ready for review?

@danepowell
Copy link
Contributor

Adam mentioned that we need to wait for approval on this.

@wimleers
Copy link
Member Author

wimleers commented Aug 8, 2023

There's absolutely no reason left that it's in draft state 😅 I should've marked this as ready for review ever since #1543 (comment) — sorry!

@wimleers wimleers marked this pull request as ready for review August 8, 2023 15:36
@wimleers
Copy link
Member Author

wimleers commented Aug 8, 2023

The actual tests are still green after merging in upstream, but:

Failing after 1s — 91.61% of diff hit (target 91.77%)

Back in June, the target was 91.56% 😬 I hope that won't hold this up? 😇

@danepowell danepowell merged commit 54ce971 into main Aug 10, 2023
18 of 20 checks passed
@danepowell danepowell deleted the app-new-from-drupal7 branch February 23, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants